-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throw errors #12
base: master
Are you sure you want to change the base?
Throw errors #12
Conversation
This commit: 1. Prevents sending messages to destroyed webContents (which result in a crash) 2. Allows registering multiple webContents for push notifications We need this because we are listening for notifcations in a specific webContents. If it crashes we create a new one and call setup again, without this change the next push notification that came in would cause a crash.
That seems to be crashing the app ref: electron/electron#11797
Can't we use NOTIFICATION_SERVICE_ERROR for this ? |
I did that at first, but realized I actually wanted the errors in the main process, since that's where I have bugsnag set to catch them. I'm happy for this not to be merged into the main project, as it's kind of an odd use case, and I'm mostly doing it to help flush out any bugs with the changes to push-receiver. FWIW we're rolling the push-receiver changes, so it'll start getting some production testing :) |
// Will be called by the renderer process | ||
ipcMain.on(START_NOTIFICATION_SERVICE, async (_, senderId) => { | ||
// Retrieve saved credentials | ||
let credentials = config.get('credentials'); | ||
// Retrieve saved senderId | ||
const savedSenderId = config.get('senderId'); | ||
if (started) { | ||
webContents.send(NOTIFICATION_SERVICE_STARTED, (credentials.fcm || {}).token); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing 'fcm' from 'credentials' object will results in error when 'credentials' object is null.
can we re-write like ((credentials && credentials.fcm) || {}).token)
@MatthieuLemoine I made this change so that we can opt-in to throwing errors. We catch these and send them to bugsnag, so it should help with catching any edge cases in the gcm code.